-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Fix -Zmir-enable-passes to detect unregistered enum names in declare_passes macro #151596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
This comment has been minimized.
This comment has been minimized.
|
Update: I changed my mind on this, see below. This is a reasonable attempt at fixing the issue. But I wonder if it's just a band-aid over the real problem? Every MIR pass module has a single pass within, except for The only downside is that this would break external tools relying on the current names. But this kind of API is not guaranteed to be stable, and the breakage is trivial to fix. @saethlin, what do you think? |
| } | ||
|
|
||
| macro_rules! pass_names { | ||
| ($mod_name:ident : $pass_name:ident { $($ident:ident),* $(,)? }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be useful to have a brief comment on each rule explaining the case being handled. I.e. the first rule deals with pass groups, the second with lone pass names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in this first rule, $pass_name would be better called $pass_group, and $ident would be better called $pass_name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed them in the commit below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking some more, I think this change is ok. I don't like how the existing macro is structured, where sometimes the thing after the colon is a pass name and sometimes it's a pass group, but fixing that seems beyond the scope of this PR.
|
r=me with the aforementioned renamings but I'll give @saethlin a chance to respond in case I'm overlooking anything. |
|
cc @clubby789, who wrote this code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM as a fix, I might look at re-doing the macro in future to be more logical
|
@sgasho: looks good. Can you squash the commits (there's no value in these particular commits being separate) and then we will be good to go. |
fac135a to
99591e6
Compare
| @@ -1,4 +1,4 @@ | |||
| //@ compile-flags: --crate-type=lib -Zmir-enable-passes=+InstSimplify | |||
| //@ compile-flags: --crate-type=lib -Zmir-enable-passes=+InstSimplify-before-inline | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that you had to change this is making a strong case for the value of this change.
I agree that breaking up the I don't have a strong opinion on the implementation, macros always look like token soup to me, but this one is of reasonable size. I mostly just care that this fixes the footgun. |
|
@bors r=nnethercote,saethlin |
…arn, r=nnethercote,saethlin Fix -Zmir-enable-passes to detect unregistered enum names in declare_passes macro related: rust-lang#150910 I fixed declare_passes macro to detect unregistered enum names ### UI Results +nightly --> before: no warnings +stage1 --> after: detect SimplifyCfg as an unknown pass <img width="591" height="199" alt="スクリーンショット 2026-01-24 23 53 41" src="https://github.com/user-attachments/assets/ddabaa58-b4c6-4e80-a3c9-f40d866db273" />
Rollup of 8 pull requests Successful merges: - #150474 (Tidy: detect ui tests subdirectory changes so `tests/ui/README.md` stays in sync) - #150572 (Improve move error diagnostic for `AsyncFn` closures) - #151596 (Fix -Zmir-enable-passes to detect unregistered enum names in declare_passes macro) - #151802 (Make `QueryDispatcher::Qcx` an associated type) - #151559 ([rustdoc] Add a marker to tell users that there are hidden (deprecated) items in the search results) - #151665 (Fix contrast ratio for `Since` element in rustodoc dark theme) - #151798 (Update `askama` to `0.15.3`) - #151800 (Subscribe myself to translation diagnostics)
related: #150910
I fixed declare_passes macro to detect unregistered enum names
UI Results
+nightly --> before: no warnings
+stage1 --> after: detect SimplifyCfg as an unknown pass